fix(ui): disable json download when can csv on Superset is not allowed#22454
fix(ui): disable json download when can csv on Superset is not allowed#22454mgorsk1 wants to merge 3 commits intoapache:masterfrom
can csv on Superset is not allowed#22454Conversation
villebro
left a comment
There was a problem hiding this comment.
LGTM, although I think the linter may complain about the long line
|
@mgorsk1 can you check if you were actually able to download the JSON data? I assume you should get an error if you don't have the right perms, and if you're not, we need to also fix this in the API. |
|
hey villebro, on a second thought I think that downloading JSON itself might be always valuable and we shall just strip it of |
|
bump @villebro |
|
Hi @mgorsk1 - CI is failing due to a linting issue: |
|
@mgorsk1 feel free to hit us up on Slack if we can help with linting. It also appears there are some conflicts to mitigate, hopefully that's straightforward-ish :) Would love to see #19535 closed. @Larissa-Rocha might also want to take a look at this PR on that note. |
26ae6fe to
c9eebfb
Compare
|
fixed linting (I hope), feel free to re-run the CI @rusackas |
|
Resolved the conflict... hopefully CI passes ;) |
|
Ugh... I can't reboot the unit tests (which were probably just flaky) for some reason, so I'm going to close/reopen this to kick-start CI (again) |
|
Unit tests seem to be failing pretty reliably... can you run/address the tests locally? I also believe that a unit tests was fixed on master not long ago, so you might get lucky with a rebase. |
|
@mgorsk1 just checking to see if you have any interest in pursuing this still. |
|
Closing and re-opening to see if anything magically changes, but unless @mgorsk1 wants to check/fix unit tests, we might wind up closing this one for good. |
294ab90 to
478699f
Compare
|
rebased @rusackas |
|
Closing/reopening to kick CI. |
|
Tests still seem to be failing on this. Any chance you can run them locally and see if you can get them to pass? I wonder if it's because the UI changed since the PR was opened. |
|
Ci is failing with:
#27236 was merged recently, so I think this PR just needs a rebase and it will be good to go. |
|
@mgorsk1 would you mind rebasing this so we can merge it? |
|
Still in need of a rebase here, I think, but I'll close/reopen again in a bit of a "hail mary" attempt. |
|
Looks like this still needs a rebase to pass CI. It's been a long while since it was touched, so I'll convert it to Draft. We may circle back and close it in time if it doesn't get any touch-ups. Please mark it as ready for review if/when it's ready, and we'll happily merge it. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #22454 +/- ##
=======================================
Coverage 69.00% 69.00%
=======================================
Files 1938 1938
Lines 75831 75831
Branches 8427 8427
=======================================
Hits 52328 52328
Misses 21334 21334
Partials 2169 2169 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Closed in favor of #28429 |

SUMMARY
Disables saving artifacts to JSON when
canDownloadCSVis not allowed. This is required to prevent leaking data from superset.can csv on Supersetis a permission aiming to prevent downloading structured data from Superset but it's not covering JSON exports. This PR aims to fix this.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION